Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new(ci): run update-kernels daily. #179

Merged
merged 4 commits into from
Oct 19, 2023
Merged

Conversation

FedeDP
Copy link
Collaborator

@FedeDP FedeDP commented Oct 12, 2023

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area ci

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Refs falcosecurity/test-infra#1255

Special notes for your reviewer:

Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 12, 2023

/hold

Copy link

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an amazing improvement! Thanks for getting this PR up @FedeDP!

leogr
leogr previously approved these changes Oct 12, 2023
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 13, 2023

Also, it would be great if we removed the kernels branch protection, and allow the update-kernels job to automatically push to the branch, instead of opening a PR.
This way, we reduce the number of PRs to be approved before drivers get built to just 1, the test-infra one.

This can be done in a follow up PR of course.

@incertum
Copy link

Also, it would be great if we removed the kernels branch protection, and allow the update-kernels job to automatically push to the branch, instead of opening a PR. This way, we reduce the number of PRs to be approved before drivers get built to just 1, the test-infra one.

This can be done in a follow up PR of course.

Ohhh I now realize how this is done. Hmm I understand we can't change it over night, but perhaps we can just invoke the kernel crawler cron job from test-infra and the kernelrelease build yamls will only exists within the CI job, no PRs or approvals at all. Any other solution that accomplishes full automation will work as well.

maxgio92
maxgio92 previously approved these changes Oct 14, 2023
Copy link
Member

@maxgio92 maxgio92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

(let's coordinate on this for unlocking it :) )

@maxgio92
Copy link
Member

I agree with @incertum, we could evaluate @FedeDP a way to avoid the "double PR" (one to update kernels.json, and one to update DBG config) pipeline, without pushing directly to long-living branches.

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 16, 2023

without pushing directly to long-living branches.

we can just invoke the kernel crawler cron job from test-infra and the kernelrelease build yamls will only exists within the CI job

Please remember that our dbg is not the only consumer of the kernel crawler output jsons.
Ie: we need to push the jsons somewhere. And this means that we can only trigger the update-dbg job on post-submit.

The only real workaround would be to let update-dbg job also crawl the latest kernels and push the list somewhere (eg: s3 bucket) and have the website over there just like we do for the drivers one.
While this would reduce number of approvals to 1 (and we could finally rebase kernel-crawler repo removing any reference to the big jsons in its history to reduce the repo size, fixing falcosecurity/test-infra#1118), i don't really like the idea that the producer of the jsons is also the consumer.
Anyway, that would work fine.
In the process, we might want to drop the last_run_distro trick since the new dbg-go impl takes a few seconds to generate dbg configs. (Otherwise we would need to push an updated last_run_distro.txt to s3 too!).

@incertum
Copy link

incertum commented Oct 16, 2023

@FedeDP thanks for the additional context!

Would it be possible to have 2 streams?

The existing workflow for all remaining use cases.

A fast runner so to say that constantly crawls and builds kernel drivers. Hard requirement would be full automation, no PRs. Something like this fast runner is what I have for custom kernels and it has never failed me.

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 17, 2023

Hard requirement would be full automation

Full automation would be a bit hard to achieve, we would need to completely trust the process; ie: it would mean that test-infra update-dbg job should directly push to master, i don't really like this.
I think that a single PR on test-infra is already good to go!

Would it be possible to have 2 streams?

I think if we move the current kernel-crawler site onto download.falco.org just like we do for drivers (and it makes sense because it gets nearer to them) and the big jsons too, we can run it daily directly from test-infra that will:

  • generate new jsons
  • push them to s3
  • use directly them to generate dbg-configs
  • open a PR against test-infra with updated dbg-configs

WDYT?
dbg-go tool needs a bit of adaption, ie: i'd add a new option to directly pass a json to generate dbg configs against.

@maxgio92
Copy link
Member

maxgio92 commented Oct 17, 2023

I'd propose an alternative option for the publishing of the kernel list.

In order to:

  • Remove a PR that in this case would be needed to approve the merge of the updated crawled kernel list file to the kernels branch, and
  • Not to remove branch protection

we could generate the list on the fly in the GH actions pipeline, and publish it directly to the GH page.

This way, we'd achieve the following:

  • The list would not reside in Git, removing risks from escalation with GH users
  • The only allowed actor with write access to the list would be the GH action of the kernel-crawler repository
  • Consumers can keep working with the crawled result as the page would be public.

The drawback I see is that:

  • The DBG configurations update (update-dbg ProwJob) would miss the trigger, that right now is based on new changes in the kernels Git branch.

But we could overcome this con by running it as a Prow cronjob. It would be less efficient, but dbg-go is smart enough to skip Driverkit configurations that have not changed (thus skipping unneeded PRs).

WDYT?

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 17, 2023

I like this idea @maxgio92 ! Much cleaner, even if we would need to move update-dbg job to cron since we would miss the post-submit trigger.
Anyway, great feedback and great idea, thank you!

@incertum
Copy link

+1 @maxgio92 thanks for the more concrete outline and yes I also don't see a problem with the planned cron jobs in this regard in test-infra.

@FedeDP FedeDP dismissed stale reviews from maxgio92 and leogr via f6f4258 October 19, 2023 09:41
@poiana poiana added size/L and removed size/XS labels Oct 19, 2023
@@ -3,35 +3,8 @@ name: Update Kernels

on:
workflow_dispatch:
inputs:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support single input anymore:

  • since dbg-go is super quick to generate dbg configs (a matter of seconds)
  • since we will run the crawler daily
    we have no more needs for it.

contents: write
pull-requests: write
contents: read
pages: write
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will directly deploy pages without pushing anything to any branch.

pull-requests: write
contents: read
pages: write
id-token: write
steps:
- name: Checkout crawler
uses: actions/checkout@v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checkout default branch that now has the index.html file.

- name: Update json lists
kernel-crawler crawl --distro="*" --arch=aarch64 > $RUNNER_TEMP/aarch64/list.json

- name: Move everything under site folder
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use site folder as root for the pages deployment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was completely copy/pasted by the kernels branch.

document.getElementById('archs').appendChild(element);
});

$.getJSON(arch+'/list.json', function(data) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now points to a local (relative) file.

// "data" : data,
"order": [[ 1, "desc" ]],
ajax: {
url: arch+'/list.json',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Instead, directly deploy pages using `update-kernels` github action.

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Massimiliano Giovagnoli <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
@maxgio92
Copy link
Member

/cc

@poiana poiana requested a review from maxgio92 October 19, 2023 11:05
Copy link
Member

@maxgio92 maxgio92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

🚀

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 19, 2023

/unhold

@FedeDP
Copy link
Collaborator Author

FedeDP commented Oct 19, 2023

Let's test this!

@poiana poiana merged commit 7c5cc13 into main Oct 19, 2023
4 checks passed
@poiana poiana deleted the new/bump_update-kernels_freq branch October 19, 2023 11:09
@FedeDP FedeDP restored the new/bump_update-kernels_freq branch October 23, 2023 13:42
@FedeDP FedeDP deleted the new/bump_update-kernels_freq branch October 23, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants